Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Refresh Token Flow #19

Merged
merged 4 commits into from
Jul 30, 2024
Merged

Conversation

jeffreyflynt
Copy link

This implements the refresh token flow.

I see the refresh token is stored along with the access token, so I modified getRefreshToken to query on the Access Token collection.

Let me know if you have any questions or suggestions.

@jankapunkt
Copy link
Member

Awesome @jeffreyflynt I will review after the weekend. @bratelefant this might be interesting to you, too

@jeffreyflynt
Copy link
Author

It looks as if there were some linting issues, I fixed those and pushed another commit.

@jankapunkt
Copy link
Member

@jeffreyflynt I'll need a bit more time because I need to complete Meteor 3.0 migration with this package

@jeffreyflynt
Copy link
Author

@jeffreyflynt I'll need a bit more time because I need to complete Meteor 3.0 migration with this package

I can make a PR for said migration as I had done this for myself if that will help; unless you have already begun.

@jankapunkt
Copy link
Member

@jeffreyflynt yes I already did that but didn't publish or push as I wanted to test locally first. I now made it public via #21 and also released leaonline:[email protected].

You can review it if you like. In the meantime I will check on your PR what I can do to get the tests fixed.

@jeffreyflynt
Copy link
Author

Thanks. I fixed the issue that was causing the tests to fail. I added id to the return of getClient. I removed that.

@jankapunkt
Copy link
Member

@jeffreyflynt I would merge this into the current branch and release another Meteor 2.x compatible version with your contribution. For the #21 there needs to be a Meteor 3 compatible implementation. If you want I can add this to #21 but I'm also fine if you want to do it

@jankapunkt jankapunkt merged commit 69af291 into leaonline:master Jul 30, 2024
4 checks passed
@MaxwelSant
Copy link

Hey @jeffreyflynt, how are you?
I wonder why we added the id field on retrieving client, but then change was rolled back: 4526117#diff-e58997fdbe2b8e4535e2dde27b344afe6f1bbf2aabecfbfdfd10727167a883a6R57
I ask because I'm having an issue on this exact point as node-oauth2-server tries to validate clientIds using field client.id but it is not matching as on client collection is saved as clientId.

@jeffreyflynt
Copy link
Author

Hi @MaxwelSant ,

I am not fully sure tbh. @jankapunkt implemented it back in, in the current open PR. You could add this code block to your code to get this working:

if (!clientDoc.id) {

@jankapunkt
Copy link
Member

Hi @MaxwelSant @jeffreyflynt the clientDoc.id is actually clientDoc.clientId but since you could theoretically save any other data, including an abitrary id value, it would get overridden by the former code.

However maybe I was thinking too strong in terms of abstraction and noone will ever come into that usecase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants